Replace declarative Video component with imperative code #10393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![replay-soc2[bot]](https://avatars.githubusercontent.com/u/60818315?s=60&v=4){kind=small}
bvaughn
commented
Mar 4, 2024
|
|
||
| return ( | ||
| <> | ||
| <div className="progress-line full" /> | ||
| <div | ||
| className="progress-line preview-max" | ||
| style={{ width: `${clamp(Math.max(hoverPercent, precachedPercent), 0, 100)}%` }} |
There was a problem hiding this comment.
I'm not sure what this "pre-cached percent" state was. I don't think we really need it. Particularly since it's only used for our internal expanded/enhanced timeline UI. So I removed it to simplify things a bit.
bvaughn
changed the title
bvaughn
commented
Mar 5, 2024
Comment on lines
+265
to
+268
| const isPlaying = getPlayback(getState()) !== null; | ||
| if (isPlaying) { | ||
| dispatch(stopPlayback()); | ||
| } |
There was a problem hiding this comment.
I think it is a reasonable change. Any objections?
| @@ -412,15 +416,15 @@ export function startPlayback( | |||
|
|
|||
| export function stopPlayback(updateTime: boolean = true): UIThunkAction { | |||
| return async (dispatch, getState) => { | |||
| dispatch(setTimelineState({ playback: null })); | |||
There was a problem hiding this comment.
This goes along with the other change, to prevent a potential loop.
bvaughn
changed the title
hbenl
approved these changes
Mar 6, 2024
markerikson
added a commit
that referenced
this pull request
Mar 25, 2024
* [RUN-3292] separately targetable steps for e2e on linux-x86_64 and macos-arm64 (#10392) Support running the e2e tests on both platforms, since we've had issues affecting only macos-arm64 in the past. In an effort to not wait on both chromium builds before starting the tests, make each step targetable using env vars. The corresponding PR in chromium (replayio/chromium#1142) waits on a single build and then triggers this pipeline with the proper env vars. This could also be done with multiple buildkite pipelines and/or multiple .yml files, but this seemed the best choice to keep things together. * Migrate more e2e examples to Chromium (#10396) * Add ReplayWall.sendAtPauseId() (#10399) * Replace declarative Video component with imperative code (#10393) Using a suspense cache for this was a mistake. Renders need to happen too quickly, and mixing rapid updates with suspense leads to starvation problems (renders that don't finish and update the DOM). I rewrote this to use imperative logic, which is how it was before (in a way) though I think this code is more consolidated. * Warn if hit counts can't be loaded for the outline panel (#10400) * Fixed timeline tooltip edge positioning (#10401) * Update @replayio/protocol to version 0.70.0 (#10402) * [RUN-3224] Remove `authenticated/comments-03` from blacklist (#10403) * Upgrade suspense (#10404) * Don't reset page title after a root-level error (#10406) * Don't try to load computed properties if no element is selected (#10409) * Show picker loading state until bounding rects are loaded (#10410) * Improve detection of DOM elements (#10411) * Redirect mobile clients to warning page (#10407) * Revert "Redirect mobile clients to warning page (#10407)" (#10412) This reverts commit e0b0aab because of FE-2346 * Remove default roles for new members (#10370) * Remove default roles for new members * fix toggling roles instead of setting them * refactor role selection for clarity * ensure viewer role is added * Redirect mobile clients to warning page (#10413) * Redirect mobile clients to warning page (FE-2342) * Make test suites test-runs 2 less flaky (FE-2346) * Improve getDimensions() method error message (#10414) * Pass testScope through GraphQL Subscription Request In order to properly scope comments we need to have the testScope available on the websocket request. Headers on websocket requests are weird, you are not really supposed to use them, so we pass it through the same way we do the auth details. This is a noop on the backend right now, but soon it will be used. * Fix iteration in TestStepSourceLocationCache (#10417) * Don't override null fallback in InlineErrorBoundary (#10416) * Remove cloning and user-reset from e2e tests (#10418) Resolves FE-2347 and BAC-4754. This was originally added in #9497 as a way to avoid parallel tests from interfering with each other but has since proved to be difficult to maintain on the backend side. Our tests will now use unique "scopes" instead to isolate comments, points, and nags between runs. I've tested this locally by... - Running tests in parallel, multiple times - Killing tests in the middle (leaving stranded comments) and then re-running them * Improve repaint/graphics e2e test stability (#10419) I noticed some repaint e2e test failures recently. They aren't common, but when they occur, it seems like our logic of waiting for a screenshot to change is not robust enough to handle a delay in loading the initial screenshot, which can cause subsequent comparisons to fail. To reduce the likelihood of this, I've updated our test helpers to explicitly wait for the current screenshot to finish loading before returning the graphics content. I think this should improve the stability of the various repaint related tests. * Fix timing problem with elements-panel helper (#10420) * Redirect Gecko recordings to legacy.replay.io (#10421) Resolves FE-2107, FE-2132, and FE-2133 - Dashboard links directly to legacy.replay.io - If a Gecko recording is opened on app.replay.io, show a redirect UI - Update e2e test recording scripts to remove "firefox" as a target - Temporarily disabled the last remaining Firefox e2e test (inspector-rules-02) * Disable repaint-01 e2e test (#10423) * Remove legacy (Gecko) Elements panel (#10424) - Delete legacy replay-next/components/elements code - Copied over the two Suspense caches and three util methods that were referenced by external code - Rename replay-next/components/elements-new to replay-next/components/elements - Update all references This is a prerequisite for making use of persistent ids in this panel. * Remove legacy (Gecko) React DevTools panel (#10425) * Rename legacy replay-next/components/elements package to replay-next/components/elements-old * Remove legacy elements panel; update references * Rename replay-next/components/elements-new to replay-next/components/elements * Udpate imperative API comment * Remove legacy, Gecko React DevTools * Harden Timeline event handlers (#10426) - Fixed a logic error in stopPlayback that caused it to null out playback state before checking if it was null (🤡) - I think this was the core of the bugfix - Move Timeline move-move and mouse-up handlers to the body element, to ensure that dragging past the timeline's boundaries updates the current time appropriately - This was making it difficult to scrub the playback time to the beginning of a recording, for example - Remove no-longer-used "dragging" state from Redux - Remove no-longer-used updateTime param to Redux stopPlayback action * Auto-redirect to legacy.replay.io (#10427) * Fix handling of Control key in KeyboardModifiersContext (#10432) * Allow internal users to create teams without trials (#10430) * Update doc_control_flow.html to include more precise logging (#10433) * Add switch accounts interstitial to login flow (#10431) * Add missing useEffect dependency (#10436) * Update TypeScript dep from 5.3.3 to 5.4.2 (latest) (#10428) * Update TypeScript dep from 5.3.3 to 5.4.2 (latest) * Remove `typescript` dependency from `replay-next` package * Fixed used semver range fro the `typescript` dependency * Remove redundant `typescript` dev dependency from packages * Revert "Remove redundant `typescript` dev dependency from packages" This reverts commit 8d38b36. * Unify TypeScript version used across all contained packages * Revert "Add switch accounts interstitial to login flow (#10431)" (#10437) This reverts commit 3c81069. * Delete unused replay-next test harness code (#10434) These views were useful in the initial development of replay-next components because they gave me isolated test harnesses. In more recent times, they were only exercised by the "Delta" screenshot tests, which were themselves removed in #10243. I also noticed we were running yarn test in that package as a GitHub Workflow, even though the main "Unit tests" workflow already includes these tests. * package.json and yarn.lock file auto changes * Fix tilt config to allow running deps in parallel (#10438) Allows devtools to be started sooner in the cloud dev flow. It currently waits until all other resources have completed before starting the devtools steps. * Revert "Update TypeScript dep from 5.3.3 to 5.4.2 (latest) (#10428)" (#10439) This reverts commit f5dc68b. * Update doc_rr_objects and doc_control_flow and re-enable repaint-01 (#10441) * Fix repaint-05 e2e test flakiness (#10443) * Re-enable the inspector-computed-02 e2e test (#10444) * Report session timeouts in e2e tests (#10445) * Update TypeScript dep from 5.3.3 to 5.4.2 (latest) - take 2 (#10442) * Revert "Revert "Update TypeScript dep from 5.3.3 to 5.4.2 (latest) (#10428)" (#10439)" This reverts commit 3ca214b. * Fixed the import style of `testFunction` in the Redux example * Add extra validation for a custom `playwrightScript` * Fix the inspect_element nag and the passport-02 test (#10446) * Add switch accounts interstitial to login flow (take 2) (#10440) * Reapply "Add switch accounts interstitial to login flow (#10431)" (#10437) This reverts commit 2a470e0. * fix handling auth requests coming from the browser * add comments * fix connection parsing; hide switch accounts on replay browser * Dont swallow error when recording e2e tests (#10447) * Reimplement some e2e tests (#10448) * Use different network request in authenticated/comments-03 test (#10450) * Add readme docs for playwright plugin test example (#10451) * Add more checks to passport-01 e2e test (#10452) * Improve paint/repaint behavior (#10449) Makes several small changes to graphics related code to improve interruption and better handle caching edge cases * Consider repaint graphics when finding the nearest paint (#10453) We have many layers of Suspense caching: 1. `PaintsCache` - caches results from one-time call to `Graphics.findPaints` (array of timestamps and paint hashes) 2. `RepaintGraphicsCache` - caches results from all calls to `DOM.repaintGraphics` (screenshots) 3. `screenshotCache` - caches results from all calls to `Graphics.getPaintContents` (screenshots for the paint hashes cached in the `PaintsCache`) 4. `paintHashCache` - caches screenshot data for paint hash (screenshots from `RepaintGraphicsCache` and `screenshotCache`) since the `DOM.repaintGraphics` API only sends screenshot data once per paint hash This commit merges cached paints and repaint data into a single, sorted list and updates all of the various _findClosest_ and _findMostRecent_ methods to use the merged data source. This results in less flickering when stepping between cached paint points. ### Before Graphics "flickers" when stepping because the most recent cached paint is displayed while a repaint is being fetched. https://github.com/replayio/devtools/assets/29597/10eb506a-abe4-443f-be08-e21ebdeccd87 ### After No "flicker" when stepping because the nearest paint is now the recently loaded repaint. https://github.com/replayio/devtools/assets/29597/87c18646-2758-4951-a321-842c209ffa29 * Add messages for the most common sources of e2e flakiness (#10454) * Update node picker e2e test logic to handle DPR > 1 (#10455) * Fix the passport-02 and inspector-elements-02 tests (#10456) * Ensure the test step details panel doesn't get stuck in loading state (#10457) * Fix selection in filtered list of tests (#10459) * Add message to addCommentHelper() (#10460) --------- Co-authored-by: Chris Toshok <ctoshok@replay.io> Co-authored-by: Holger Benl <hbenl@evandor.de> Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com> Co-authored-by: Domi <Domiii@users.noreply.github.com> Co-authored-by: Ryan Duffy <ryan@replay.io> Co-authored-by: Josh Morrow <josh@jcmorrow.com> Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using a suspense cache for this was a mistake. Renders need to happen too quickly, and mixing rapid updates with suspense leads to starvation problems (renders that don't finish and update the DOM). I rewrote this to use imperative logic, which is how it was before (in a way) though I think this code is more consolidated.
Please give extra thought to the two small changes I made in
actions/timeline(I left comments) as well as the removal of theprecachedPercentRedux state. I think these changes are sound but I would like the reviewer to give them consideration as well.